Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(express): listen for finish event on response for async express layer #107 #188

Merged
merged 4 commits into from
Sep 30, 2020

Conversation

vmarchaud
Copy link
Member

I was hesitant of implement it like this from the start (for performance reason since we are adding a listener for every async layer) but seeing the half-broken (timing are not accurate) spans reporting, i think it's better like this for end user.

@vmarchaud vmarchaud added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 29, 2020
@vmarchaud vmarchaud requested a review from a team August 29, 2020 11:42
@vmarchaud vmarchaud self-assigned this Aug 29, 2020
@codecov
Copy link

codecov bot commented Aug 31, 2020

Codecov Report

Merging #188 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   95.21%   95.26%   +0.05%     
==========================================
  Files          92       87       -5     
  Lines        4723     4541     -182     
  Branches      488      476      -12     
==========================================
- Hits         4497     4326     -171     
+ Misses        226      215      -11     
Impacted Files Coverage Δ
node/opentelemetry-plugin-express/src/express.ts 97.02% <0.00%> (-0.93%) ⬇️
node/opentelemetry-plugin-redis/src/version.ts
node/opentelemetry-plugin-redis/src/redis.ts
node/opentelemetry-plugin-redis/test/redis.test.ts
node/opentelemetry-plugin-redis/.eslintrc.js
node/opentelemetry-plugin-redis/src/utils.ts
.../opentelemetry-plugin-express/test/express.test.ts 99.28% <0.00%> (+<0.01%) ⬆️

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just have few question to understand the changes better

@vmarchaud
Copy link
Member Author

@obecny Sorry for the force push, i didn't saw your review and wanted to make a review before someone would do one :/

@obecny obecny added the bug Something isn't working label Sep 7, 2020
@vmarchaud
Copy link
Member Author

This PR has been open for a while now and it contains a important bug fix, can we merge it or do we need more review ?

@vmarchaud
Copy link
Member Author

I think we can merge this one ? cc @obecny @dyladan

@obecny obecny merged commit ed0edf8 into open-telemetry:master Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants